Conversation
|
This still needs:
Though I might land this before steps 2 and 3 just to verify the fallback is working. |
56430bf to
94eff61
Compare
| github_repo = GitHubRepository(owner=repo_definition["parent"]["owner"]["login"], repo_name=repo_definition["parent"]["name"], token=token) | ||
| github_repo = GitHubRepository(context, repo_definition["parent"]["owner"]["login"], repo_definition["parent"]["name"]) |
There was a problem hiding this comment.
Isn't this going to be a problem? If I open a PR from github.com/Eijebong/foo for github.com/mozilla-releng/foo, the task won't have scopes to get a read token for Eijebong/foo and the tc-auth token request will fail 100% of the time.
I'm not sure how we can do that but we probably want to use a read token minted for the parent repo and use that instead? AFAIK that'd work for public repos but not private ones though (although all fork commits are accessible on the parent directly, maybe that's enough to make this whole branch useless?).
There was a problem hiding this comment.
Thinking more about this, I think the private repo part of this is the same anyway since the token passed a secret wouldn't have access to it either
There was a problem hiding this comment.
Good catch.
As implemented this won't be an immediate problem because we fallback to github_oauth_token if there's a problem fetching the token from the auth service.
But assuming the goal is to get rid of github_oauth_token, we'd need to implement something like this.
I'm thinking of leaving the github_oauth_token fallback for this PR, but add a comment to make sure we don't forget about the fork case when we eventually go to remove it?
a01199e to
1afe69b
Compare
This is the first version that implements the auth service's ability to hand out Github tokens.
…possible If there's an error (e.g missing scopes or app not installed on the target repo), we fallback to the existing github_oauth_token.
|
Here's a task demonstrating the fallback to GITHUB_OAUTH_TOKEN working: And here it is with scopes, but still hitting the fallback due to the app not being installed: And here it is using the auth service token: (but still using the fallback on a docker-image task that came from the upstream repo because I didn't grant the scopes / install the app there) |
1afe69b to
d094e19
Compare
|
One thing I'm undecided about.. do we want to print the full Taskcluster failure message including the regular scope error? On the one hand it's pretty noisy, but noisy might be a good thing. CoT makes a lot of requests and we already started exceeding |
|
And by eventually I mean sooner rather than later |
This branch has not been deployed
2 participants
No description provided.